Optimization for concurrent Key Vault Ref resolution - deduplicate requests to the same secret#328
Optimization for concurrent Key Vault Ref resolution - deduplicate requests to the same secret#328linglingye001 wants to merge 4 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds in-flight request deduplication to Key Vault secret resolution so that concurrent configuration settings referencing the same secret URI don’t trigger redundant Key Vault (or resolver) calls.
Changes:
- Added an in-flight promise map in
AzureKeyVaultSecretProvider.getSecretValue()to deduplicate concurrent secret fetches by secret identifier. - Added test coverage validating deduplication in parallel and sequential modes, secret-version independence, failure behavior, and refresh behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/keyvault/keyVaultSecretProvider.ts |
Introduces in-flight request deduplication for identical secret identifiers and ensures in-flight entries are cleared after completion. |
test/keyvault.test.ts |
Adds a new test suite covering request deduplication behavior across parallel/sequential resolution, versions, failures, and refresh rounds. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| await sleepInMs(60_000 + 100); | ||
| await settings.refresh(); |
linglingye001
force-pushed
the
linglingye/dedup-key-vault-ref
branch
from
03023cd to
0e635bb
Compare
| // Resolve every reference. In parallel mode the values are served from the warmed cache | ||
| // without any additional I/O. | ||
| for (const setting of secretReferences) { | ||
| const [key, value] = await this.#processKeyValue(setting); | ||
| resultHandler(key, value); | ||
| } |
| if (secretId === undefined) { | ||
| // The reference cannot be parsed; resolve it individually so the appropriate error | ||
| // is surfaced when it is processed below. | ||
| uniqueSecretReferences.push(setting); | ||
| } else if (!seenSecretIds.has(secretId)) { | ||
| seenSecretIds.add(secretId); | ||
| uniqueSecretReferences.push(setting); | ||
| } |
| // Return the cached value if available. The cache is invalidated externally (on secret refresh | ||
| // or when a key-value change is detected) so a stale value is never served. |
| // After the secret refresh interval elapses, the refresh round re-fetches once. | ||
| await sleepInMs(60_000 + 100); | ||
| await settings.refresh(); |
| } | ||
|
|
||
| // Invalidate cached secret values so the refresh round re-fetches them from Key Vault. | ||
| this.#keyVaultAdapter.clearCache(); |
There was a problem hiding this comment.
With the introduction of features such as secret refresh, parallel loading, and deduplication, AppConfigurationImpl now needs more fine-grained control over secret loading and caching.
When KeyValueAdapter.onChangeDetected is available, but we bypass it and directly call SecretProvider or Adapter.clearCache, I do not think the existing KeyVaultKeyValueAdapter abstraction remains a sound design.
There was a problem hiding this comment.
I think we can introduce preload(settings: ConfigurationSetting[]): Promise<void>; to IKeyVaultAdapter
This method will dedup and warm the secret cache.
SecretProvider needs to be updated. getSecretValue should now only serve secret value from cache (if no cache is found, fallback to send request), the timer gate should be removed.
The timer gate should apply for the preload path only.
| * Optionally batch-resolves settings ahead of processKeyValue, e.g. to deduplicate and warm up | ||
| * Key Vault secret requests so that processKeyValue only reads from cache. | ||
| */ | ||
| preload?(settings: ConfigurationSetting[]): Promise<void>; |
There was a problem hiding this comment.
This method should not be optional. We should make it and its comment consistent with OnChangeDetected.
There was a problem hiding this comment.
In the comment we should explain the parameter: whether it should be just a bunch of configuration settings or a set of certain type of configuration settings that the adapter can process
There was a problem hiding this comment.
I actually prefer to let preload only take settings that the adapter can process. This will match the orchestrator(appConfigurationImpl) + kv adapter design.
The orchestrator should call preload like this:
for (const adapter of this.#adapters) {
await adapter.preload.(settings.filter(s => adapter.canProcess(s)));
}The above code has very clear intention.
But the concern here is that it will increase the cost to MxN (M is adapater count, N is settings count)
There was a problem hiding this comment.
With that concern, I think maybe we should make preload and onChangeDetected optional method under the IKeyValueAdapter to leverage the JS/TS language characteristics for better performance.
| */ | ||
| async preload(settings: ConfigurationSetting[]): Promise<void> { | ||
| if (!this.#keyVaultOptions) { | ||
| return; // nothing to do; processKeyValue will throw the proper ArgumentError |
There was a problem hiding this comment.
| return; // nothing to do; processKeyValue will throw the proper ArgumentError | |
| return; // no-op when keyVaultOptions is not configured |
| * timer and the parallel resolution option. This is best-effort: per-secret failures are swallowed so | ||
| * that the error is surfaced with full context later by getSecretValue during resolution. | ||
| */ | ||
| async preloadSecrets(secretIdentifiers: KeyVaultSecretIdentifier[]): Promise<void> { |
There was a problem hiding this comment.
preload is the concept from the abstraction of kv adapter. This business logic should be placed in adapter implementation.
I think secret provider should offer two methods: 1. loadSecretValue 2. getSecretValue
loadSecretValue has refresh timer gate, if timer is not due, serve secret value from cache, when there is no cache or timer is due, the fallback is to send request to key vault to get secret value
getSecretValue simply serve secret value from cache, when there is no cache, the fallback is loadSecretValue
No description provided.